Skip to content

refactor: add batch content validation #1869

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
May 31, 2025
Merged

refactor: add batch content validation #1869

merged 2 commits into from
May 31, 2025

Conversation

morph-dev
Copy link
Collaborator

@morph-dev morph-dev commented May 30, 2025 •
edited
Loading

This was extracted from #1868

What was wrong?

Validator is currently able to validate only one content at the time. This is not enough for ephemeral headers.
Similar for Store when answering whether content is/should be stored.

How was it fixed?

Rename ContentStore::is_key_within_radius_and_unavailable to ContentStore::should_we_store and add Store::should_we_store_batch (for multiple items.

Add Validator::validate_content_batch for validating multiple content items.

Refactor ValidationResult:

  • removed additional_content_to_propagate from ValidationResult (it's not used)
  • made ValidationResult an enum
    • enum type works better as a result of validate_content_batch, because it is Vec<ValidationResult> instead of Vec<Result<ValidationResult>>
    • we can consider in the future changing Invalid(String) variant into more descriptive type, e.g. Invalid(ValidationFailureReason) where: 
      enum ValidationFailureReason {
    ContentValueNotDecodable,
    Invalid(<reason?>),  // e.g. merkle proof not valid
    FetchingAnchorContentFailuer(<reason?>), // e.g. failed to fetch Header when verifying BlockBody
    ...
}

Future work

Start using batch functions from offer.rs (#1870) and add custom implementation of the batch functionality for history network (future PR).

@morph-dev morph-dev requested review from KolbyML and carver May 30, 2025 18:39
@morph-dev morph-dev self-assigned this May 30, 2025
This was referenced May 30, 2025
Merged
Closed
KolbyML
KolbyML approved these changes May 31, 2025
View reviewed changes
Copy link
Member

@KolbyML KolbyML left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit: looks good for the most part, I don't know why we don't push up and now hide the decode errors in a few spots, but other then that the PR is clean

@morph-dev
Copy link
Collaborator Author

morph-dev commented May 31, 2025 •
edited
Loading

Since all the comments are on the same topic, I will just reply here.

I don't think that logging or returning ssz::DecodeError is useful. I mean, it's useful to know that it's decoding that failed but the debug message of the failure is in my opinion not useful at all because:

  • It will just say something like: InvalidByteLength { len: 1000, expected: 104 } or OutOfBoundsByte { i: 1000 } which isn't useful at all
  • And even if you would try to understand the actual message and what specific byte is wrong (which I don't see a reason for doing anyway), the error message can come from decoding any of the inner fields (or inner-inner fields) and offset would be very off so you wouldn't be able to do it anyway.

So I decided to just return error that indicates that error happened because of the decoding (and not because of e.g. validation).

I now added debug!(...) log with the actual content bytes that decoding failed for, which would actually be useful if somebody wants to debug the issue.

I'm not too set on this. If you think we should propagate error I can do so, but I would like to hear the argument for it.

@morph-dev morph-dev force-pushed the refactor_validator branch from eef8d1e to 9a420f3 Compare May 31, 2025 19:19
@KolbyML
Copy link
Member

KolbyML commented May 31, 2025

Would we be able to embeded the debugging context in the ContentValidation::Invalid() instead of defining a debug! Message?

I am not too picky I think logging it is definitely an improvement regardless. Up to you. It seems wasteful to have both though

@KolbyML
Copy link
Member

KolbyML commented May 31, 2025 •
edited
Loading

I am fine not propagating the error in this case, I agree what you did it higher value, my question is would anybody ever have the debug logs enabled.

@KolbyML
Copy link
Member

KolbyML commented May 31, 2025

Anyways I am fine with whatever you decide, if it is a problem we can always change it later of course

@morph-dev
Copy link
Collaborator Author

Would we be able to embeded the debugging context in the ContentValidation::Invalid() instead of defining a debug! Message?

I don't want to embed debug message inside Invalid(..) variant as content_value can be very big. That's why I think it should only be used as part of the debug! message.

I am fine not propagating the error in this case, I agree what you did it higher value, my question is would anybody ever have the debug logs enabled.

Our prod nodes have debug enabled.

If somebody is debugging this bug, they would enable it. Otherwise, it's probably not useful anyway.

I will leave it as is, and we can change it in the future if needed.

@morph-dev morph-dev merged commit 2145d9e into ethereum:master May 31, 2025
16 checks passed
@morph-dev morph-dev deleted the refactor_validator branch May 31, 2025 21:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@carver carver Awaiting requested review from carver

1 more reviewer

@KolbyML KolbyML KolbyML approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@morph-dev morph-dev

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@morph-dev @KolbyML